-
Notifications
You must be signed in to change notification settings - Fork 825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add AWS presigned URL support #4876
Conversation
f39e63e
to
eea6f17
Compare
eea6f17
to
6a6b556
Compare
Hm. I'm trying to put this into use in IOx and running into a bit of a problem... ideally, we'd like to not have to change much about the rest of the codebase that uses The problem with that I'm running into is that we're not actually using The solutions I'm seeing to this need to be done in
I'm going to try going down these paths to see if I run into different issues, but I'd love thoughts on the best way to solve this from the |
I think I might be missing some context on why the pre-signed URL functionality needs to be conflated with the ObjectStore trait. I would have perhaps expected a Or in other words you'd just plumb the two separately and nothing would need to know that they potentially are the same thing underneath? |
TL;DR IOx; I'm happy to make changes to IOx to do whatever. That's purely due to the current IOx implementation that has a For the bulk import functionality, what we've planned for a piece of it is a gRPC service implemented in the router that has an action that returns a pre-signed URL. Currently, the router uses the aforementioned
Right, but who defines Do you have any general comments about this PR as-is? |
I need to sleep on it, but perhaps we just add it to the ObjectStore trait with a default not implemented impl, it wouldn't be the weirdest thing there - I'm looking at you append 😅 |
Codes looks good, will continue to think about how we expose this |
Ok, so here's another wrinkle. The URL signing API should return a full URL so that if the user wants to, they can pass the URL to something that isn't using the However, if the user is doing all upload/download through the So I think the In IOx terms, the router will have something that can produce signed urls (configured using the code in clap_blocks, I think I've figured out a good way to do that) and will pass that full URL to a bulk import tool, that will also have an object store configured using the code in clap_blocks (that currently returns an Does that sound right to you? |
This looks sensible to me 👍
The upload request is a simple PUT request with the object payload as the body. I'm not sure I follow why this would benefit from being ObjectStore-ified, or what the clap_blocks configuration would be adding? |
As Raphael and I were chatting, I realized this statement of mine was exactly wrong, as Raphael also commented-- the whole point of the thing using the signed URLs is that it DOESN'T have the |
993060f
to
66f481a
Compare
Ok, I think I'm happy-ish with this? I have an iox branch working that I think isn't terrible, maybe? CI is failing with something about ring, I don't know why :( |
Ring failure was likely briansmith/ring#1707 and should be fixed by re-rerunning CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me
Co-authored-by: Raphael Taylor-Davies <[email protected]>
3bf2880
to
ba39baf
Compare
ba39baf
to
4dc20e6
Compare
@tustvold fixed, and CI is passing now!! 🎉 |
Ok, I'm feeling pretty good about this-- I did test it against a real live S3 bucket and I was able to upload a file with cURL using signed URLs generated by these new functions!
There's always more refactoring that could be done... the private method where I told clippy to let me have many arguments points to the worst of it.
If you take a look commit-by-commit, the last commit adds a public
signed_url
method directly toAmazonS3
, which I think makes it super convenient, but might be more than you'd like to expose. I'm happy to back that commit out!Which issue does this PR close?
Closes #4763.
Connects to #3027, which is requesting signed GET URLs for all object store implementations that support it-- this PR implements support for both GET and PUT signed URLs, but only for AWS.
Rationale for this change
Enable reusing the authorization and signing code to generate signed URLs that can be used to give permission to upload or view particular objects.
What changes are included in this PR?
A new public method on
AwsAuthorizer
, currently calledsign
(as opposed to the existingauthorize
method) that adds AWS signature query parameters to a specifiedUrl
so that it can be given to another user to authorize uploading or viewing a particular resource.This shares a lot of the signing code with the existing
authorize
method, so I've pulled out a few private methods, but I don't think the current structure is ideal yet. Thoughts on how far to take this are very welcome, such as if there's another struct or three starting to cry to be let out ofAwsAuthorizer
, if these should be free functions, etc.Are there any user-facing changes?
Yes, one (or possibly two) new method(s) in the public API. See the included examples!